- 
          
 - 
                Notifications
    
You must be signed in to change notification settings  - Fork 1.9k
 
π bug: Handle Unix sockets in adaptor middleware #3760
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
π bug: Handle Unix sockets in adaptor middleware #3760
Conversation
| 
           Thanks for opening this pull request! π Please check out our contributing guidelines. If you need help or want to chat with us, join us on Discord https://gofiber.io/discord  | 
    
| 
          
 Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds helpers to normalize and resolve remote addresses (including Unix domain sockets) in the adaptor, replaces inline port-fix logic with the new resolver, and adds tests for Unix socket handling and malformed RemoteAddr fallback. Changes
 Sequence Diagram(s)sequenceDiagram
  autonumber
  participant Client
  participant NetHTTP as "net/http Server"
  participant Adaptor
  participant Fasthttp as "fasthttp app"
  Client->>NetHTTP: HTTP request (TCP or Unix socket)
  NetHTTP->>Adaptor: forward http.Request (RemoteAddr, LocalAddrContextKey)
  Adaptor->>Adaptor: resolveRemoteAddr(RemoteAddr, LocalAddrContextKey)
  alt Resolved to TCP addr
    Adaptor->>Fasthttp: forward request with resolved TCP remoteAddr
  else Unix socket or unresolved
    Note right of Adaptor #E8F8F5: set remoteAddr = nil (fallback)
    Adaptor->>Fasthttp: forward request without TCP remoteAddr
  end
  Fasthttp-->>Adaptor: response
  Adaptor-->>NetHTTP: http.Response
  NetHTTP-->>Client: HTTP/1.1 response
    Estimated code review effortπ― 3 (Moderate) | β±οΈ ~20 minutes Suggested reviewers
 Poem
 Pre-merge checks and finishing touchesβ Failed checks (1 warning)
 β Passed checks (4 passed)
 β¨ Finishing touches
 π§ͺ Generate unit tests
 π Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro π Files selected for processing (1)
 π§ Files skipped from review as they are similar to previous changes (1)
 Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment   | 
    
          Summary of ChangesHello @Dishank-Sen, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a critical bug in the  Highlights
 Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either  
 Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a  Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with π and π on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
  | 
    
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request correctly fixes an issue where requests over Unix domain sockets would fail. The change in adaptor.go properly handles RemoteAddr for non-TCP connections by falling back to a nil address, which fasthttp can handle. The newly added test in adaptor_test.go provides good coverage for this scenario. I have a couple of suggestions to make the new test more robust and isolated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
π§Ή Nitpick comments (4)
middleware/adaptor/adaptor_test.go (3)
704-716: Use test logging instead of printing to stdout.Replace fmt.Println with t.Logf to keep test output clean.
// Read response buf := make([]byte, 1024) n, err := conn.Read(buf) if err != nil { listener.Close() t.Fatal(err) } rawResponse := string(buf[:n]) - fmt.Println("Raw response:\n", rawResponse) + t.Logf("Raw response:\n%s", rawResponse)
695-702: Consider a bounded dial to avoid hangs.Use net.DialTimeout with a short timeout to prevent a stuck read if the server doesnβt accept in time.
Example:
conn, err := net.DialTimeout("unix", socketPath, 2*time.Second)Requires: import "time"
717-721: Assert status code too, not only body substring.A quick check avoids false positives if body changes.
if !strings.HasPrefix(rawResponse, "HTTP/1.1 200") { t.Fatalf("unexpected status line: %q", rawResponse) }middleware/adaptor/adaptor.go (1)
171-179: Solid Unix-socket fallback; simplify and avoid err shadowing.Current logic is correct. You can simplify and make the intent clearer by using net.Addr directly and a separate tcpErr variable.
Please confirm fasthttp.RequestCtx.Init tolerates a nil remoteAddr across the supported fasthttp versions in this repo.
- // Determine remoteAddr for TCP or fallback for Unix sockets - var remoteAddr *net.TCPAddr - tcpAddr, err := net.ResolveTCPAddr("tcp", r.RemoteAddr) - if err != nil { - // Unix socket or invalid address - remoteAddr = nil // fasthttp will handle nil correctly - } else { - remoteAddr = tcpAddr - } + // Determine remoteAddr for TCP or fallback for Unix sockets + var remoteAddr net.Addr // nil => fasthttp handles it + if addr, tcpErr := net.ResolveTCPAddr("tcp", r.RemoteAddr); tcpErr == nil { + remoteAddr = addr + }
π Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
π Files selected for processing (2)
middleware/adaptor/adaptor.go(1 hunks)middleware/adaptor/adaptor_test.go(2 hunks)
π§° Additional context used
π Path-based instructions (1)
**/*.go
π CodeRabbit inference engine (AGENTS.md)
**/*.go: Format Go code using gofumpt (enforced viamake format)
Ensure code passes golangci-lint checks (enforced viamake lint)
Optimize struct field alignment using betteralign (enforced viamake betteralign)
Modernize Go code using gopls modernize (enforced viamake modernize)
Files:
middleware/adaptor/adaptor.gomiddleware/adaptor/adaptor_test.go
π§ Learnings (1)
π Learning: 2024-11-10T23:44:13.704Z
Learnt from: gaby
PR: gofiber/fiber#3193
File: middleware/adaptor/adaptor.go:111-111
Timestamp: 2024-11-10T23:44:13.704Z
Learning: In the `middleware/adaptor/adaptor.go` file of the Fiber framework, when updating context handling, replacing `c.Context()` with `c.RequestCtx()` is appropriate to access the `fasthttp.RequestCtx`.
Applied to files:
middleware/adaptor/adaptor.go
𧬠Code graph analysis (1)
middleware/adaptor/adaptor_test.go (1)
middleware/adaptor/adaptor.go (1)
FiberApp(137-139)
          Codecov Reportβ Patch coverage is  
 Additional details and impacted files@@            Coverage Diff             @@
##             main    #3760      +/-   ##
==========================================
+ Coverage   91.50%   91.54%   +0.03%     
==========================================
  Files         113      113              
  Lines       11896    11912      +16     
==========================================
+ Hits        10886    10905      +19     
+ Misses        742      740       -2     
+ Partials      268      267       -1     
 Flags with carried forward coverage won't be shown. Click here to find out more. β View full report in Codecov by Sentry. π New features to boost your workflow:
  | 
    
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
π§Ή Nitpick comments (2)
middleware/adaptor/adaptor_test.go (2)
699-705: Guarantee server shutdown even on early failures via t.Cleanup.This makes teardown robust and avoids relying on manual closes sprinkled across error paths.
Apply this diff:
done := make(chan struct{}) go func() { if err := http.Serve(listener, handler); err != nil && err != http.ErrServerClosed { t.Errorf("http.Serve failed: %v", err) } close(done) }() + t.Cleanup(func() { + _ = listener.Close() + <-done + })Optional: once this is added, you can remove the explicit
listener.Close()and<-doneat Lines 744-746.
707-718: Set a connection deadline to prevent hangs/flakes.Without a deadline, a stalled read/write can hang the test. Add a short deadline.
Apply this diff:
conn, err := net.Dial("unix", socketPath) if err != nil { if err := listener.Close(); err != nil { t.Errorf("listener.Close failed: %v", err) } t.Fatal(err) } defer func() { if err := conn.Close(); err != nil { t.Errorf("conn.Close failed: %v", err) } }() + // avoid indefinite hangs on read/write + require.NoError(t, conn.SetDeadline(time.Now().Add(5*time.Second)))And add the missing import:
import ( "bytes" "context" "fmt" "io" "net" "net/http" "net/http/httptest" "net/url" "os" "path/filepath" "runtime" + "time" "strings" "testing"
π Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
π Files selected for processing (1)
middleware/adaptor/adaptor_test.go(2 hunks)
π§° Additional context used
π Path-based instructions (1)
**/*.go
π CodeRabbit inference engine (AGENTS.md)
**/*.go: Format Go code using gofumpt (enforced viamake format)
Ensure code passes golangci-lint checks (enforced viamake lint)
Optimize struct field alignment using betteralign (enforced viamake betteralign)
Modernize Go code using gopls modernize (enforced viamake modernize)
Files:
middleware/adaptor/adaptor_test.go
𧬠Code graph analysis (1)
middleware/adaptor/adaptor_test.go (1)
middleware/adaptor/adaptor.go (1)
FiberApp(137-139)
| 
           /gemini review  | 
    
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request correctly fixes a bug that prevented the adaptor middleware from handling requests over Unix domain sockets. The change in adaptor.go is sound, and the new unit test in adaptor_test.go provides excellent coverage for this functionality. I've included one suggestion to refactor the new test for improved robustness and code clarity. Overall, this is a solid contribution.
| 
           @gaby All tests are passing on this PR. Just wanted to kindly ask if thereβs anything else needed from my side to move this forward, or if itβs ready for review/merge. Thanks for your time .  | 
    
| 
           @Dishank-Sen Will merge this today  | 
    
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and canβt be posted inline due to platform limitations.
β οΈ  Outside diff range comments (1)
middleware/adaptor/adaptor.go (1)
197-205: Remove unsafe SplitHostPort type-assert; rely on resolveRemoteAddrDrop the pre-mutation of
r.RemoteAddr:resolveRemoteAddralready handles missing ports and avoids a panic on non-AddrError.- if _, _, err := net.SplitHostPort(r.RemoteAddr); err != nil && err.(*net.AddrError).Err == "missing port in address" { //nolint:errorlint,forcetypeassert,errcheck - r.RemoteAddr = net.JoinHostPort(r.RemoteAddr, "80") - } - - remoteAddr, err := resolveRemoteAddr(r.RemoteAddr, r.Context().Value(http.LocalAddrContextKey)) + remoteAddr, err := resolveRemoteAddr(r.RemoteAddr, r.Context().Value(http.LocalAddrContextKey))
π§Ή Nitpick comments (2)
middleware/adaptor/adaptor.go (2)
142-149: Solid Unix-network detection; one-line simplification possibleThe switch works. Tiny nit: a single return reads cleaner.
-func isUnixNetwork(network string) bool { - switch network { - case "unix", "unixgram", "unixpacket": - return true - default: - return false - } -} +func isUnixNetwork(network string) bool { + return network == "unix" || network == "unixgram" || network == "unixpacket" +}
152-156: Avoid re-parsing Unix addrs; return the existing addrIf LocalAddrContextKey is a Unix addr, you can return it directly instead of creating a new one from String(); this avoids odd βunix:β¦β names and unnecessary work.
-func resolveRemoteAddr(remoteAddr string, localAddr any) (net.Addr, error) { - if addr, ok := localAddr.(net.Addr); ok && isUnixNetwork(addr.Network()) { - return net.ResolveUnixAddr("unix", addr.String()) - } +func resolveRemoteAddr(remoteAddr string, localAddr any) (net.Addr, error) { + if addr, ok := localAddr.(net.Addr); ok && isUnixNetwork(addr.Network()) { + // Pass through the Unix addr as-is. + return addr, nil + }
π Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
π Files selected for processing (1)
middleware/adaptor/adaptor.go(3 hunks)
π§° Additional context used
π Path-based instructions (1)
**/*.go
π CodeRabbit inference engine (AGENTS.md)
**/*.go: Format Go code using gofumpt (enforced viamake format)
Ensure code passes golangci-lint checks (enforced viamake lint)
Optimize struct field alignment using betteralign (enforced viamake betteralign)
Modernize Go code using gopls modernize (enforced viamake modernize)
Files:
middleware/adaptor/adaptor.go
π§ Learnings (1)
π Learning: 2024-11-10T23:44:13.704Z
Learnt from: gaby
PR: gofiber/fiber#3193
File: middleware/adaptor/adaptor.go:111-111
Timestamp: 2024-11-10T23:44:13.704Z
Learning: In the `middleware/adaptor/adaptor.go` file of the Fiber framework, when updating context handling, replacing `c.Context()` with `c.RequestCtx()` is appropriate to access the `fasthttp.RequestCtx`.
Applied to files:
middleware/adaptor/adaptor.go
π Additional comments (1)
middleware/adaptor/adaptor.go (1)
201-205: Good fallback for Unix socketsGracefully defaulting to nil remoteAddr lets fasthttp proceed without a 500 on Unix sockets. Nice.
| resolved, err := net.ResolveTCPAddr("tcp", remoteAddr) | ||
| if err == nil { | ||
| return resolved, nil | ||
| } | ||
| 
               | 
          ||
| var addrErr *net.AddrError | ||
| if errors.As(err, &addrErr) && addrErr.Err == "missing port in address" { | ||
| remoteAddr = net.JoinHostPort(remoteAddr, "80") | ||
| return net.ResolveTCPAddr("tcp", remoteAddr) | ||
| } | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
π οΈ Refactor suggestion | π Major
Missing-port handling is robust here
Good use of errors.As and fallback to :80. Recommend consolidating on this path and deleting the older SplitHostPort/type-assert block below to avoid duplication and unsafe casts.
π€ Prompt for AI Agents
In middleware/adaptor/adaptor.go around lines 157 to 166, the new
errors.As-based missing-port handling correctly falls back to port 80; remove
the older SplitHostPort/type-assert block further down (the duplicated logic
that uses net.SplitHostPort and unsafe type assertions) so there is a single,
robust path for handling addresses missing a port; ensure no remaining
references to the deleted variables or casts remain and that net.ResolveTCPAddr
is used consistently for both the normal and fallback cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Dishank-Sen! I noticed there were some test coverage gaps and also there is a potential panic issue in the CopyContextToFiberContext function (outside of the diff range). I've been working on some improvements locally - would you be interested in collaborating? I can either:
- Share the fixes with you so you can add them to your branch
 - Create a separate follow-up PR after yours is merged
 - Help you implement the fixes or commit them to this branch
 
What would work best for you?
| 
           @sixcolors Iβd be happy to collaborate. Feel free to share the fixes and Iβll add them, or open a follow-up PR β whichever you prefer.  | 
    
| 
           unix-socket-improvements.patch Was before 4d25336, but contained coverage that it included as well as coverage for other missing lines outside of the diff range.  | 
    
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved. If the suggestions in the patch arenβt adopted, Iβll create a follow-on PR and tag @Dishank-Sen for review.
| 
           @Dishank-Sen Did you apply the changes from @sixcolors patch?  | 
    
| 
           @gaby As of my last commit, I havenβt applied the changes from @sixcolorsβ patch yet.  | 
    
| 
           Congrats on merging your first pull request! π We here at Fiber are proud of you! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord  | 
    
* Fix Fiber v3 adapter for Unix socket testing * test(adaptor): use temp dir for Unix socket and skip on non-Unix * formatted test file and handle error check * resolves issues in adapter test * *net.TCPaddr to net.addr * fixes in adaptor test * fixes in adaptor test * fixes in adaptor test * checked error return in adataptor test * resolved variable shadowing in adaptor test * resolved variable shadowing in adaptor test * resolved variable shadowing in adaptor test * added resolveRemoteAddr function for resolving addr properly * changes in resolveRemoteAddr function * resolved lint error * added test for bad remote address * resolve lint issue in adaptor test * resolve lint issue in adaptor test
PR Title
π Fix adaptor.FiberApp to handle Unix sockets + β add unit test
Description
This PR fixes a bug in
adaptor.FiberAppthat caused requests over Unix domain sockets to return a500 Internal Server Error.The issue was due to improper handling of
RemoteAddrfor Unix socket connections. A fallback mechanism has been added to correctly handle requests via Unix sockets.Additionally, a new unit test has been added to validate Unix socket support.
Changes introduced
adaptor.FiberAppto properly handleRemoteAddrfor Unix socket requests.unixsocket_test.goto validate Unix domain socket functionality.Type of change
Checklist
Fixes #3751